@sveltejs/kit#2
Merged
Merged
Conversation
add git pre push hook for mimicking GitHub Actions CI steps
bump version 2.49.4-rc.0
…igure fork metadata rename to @tg-svelte/kit
This commit addresses several regressions identified in the client-side initialization script generation ([generate_init_script](cci:1://file:///home/shane/repos/svelte-kit/packages/kit/src/runtime/server/page/render.js:341:2-561:4)), which were caused by the `legacy_support_and_export_init` path omitting logic present in the main (unused) code path. Changes: - **Service Workers**: Ported missing logic to correctly serialize and inject `service_worker_options` into the initialization script. - **Streaming**: Implemented robust [resolve](cci:1://file:///home/shane/repos/svelte-kit/packages/kit/src/exports/vite/index.js:670:2-672:3) logic in the init script that handles dynamic `app` imports and correct function signatures (`fn(app)`), ensuring streaming works correctly with custom data types. - **Serialization**: Moved the initialization of `data` inside the `Promise.all()` callback. This ensures that the `app` module is fully loaded and available before `data` is evaluated, preventing errors when `data` involves custom transport decoding (e.g., `app.decode()`). - **Tests**: Patched the `request-abort` test in the `dev-only` app with a small delay to prevent race conditions where the verification request could race the server's abort signal processing.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 479 files
Confidence score: 5/5
- Only noted issue is a minor typo in
documentation/legacy/PR_DRAFT.md, so overall risk is very low. - Safe to merge with confidence; the change is purely documentation-level.
- Pay close attention to
documentation/legacy/PR_DRAFT.md- spelling fix in description.
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="documentation/legacy/PR_DRAFT.md">
<violation number="1" location="documentation/legacy/PR_DRAFT.md:10">
P3: Fix the spelling error in the description so it reads “properties like `bundle.output`.”</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
gamesguru
changed the title
find . -type f \( -name "*.js" -o -name "*.ts" -o -name "*.svelte" -o -name "*.md" -o -name "*.json" \) -not -path "*/node_modules/*" -not -path "*/.git/*" -not -path "*/.changeset/*" -print0 | xargs -0 sed -i 's/@sveltejs\/kit/@tg-svelte\/kit/g'
There was a problem hiding this comment.
11 issues found across 54 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="documentation/docs/25-build-and-deploy/80-adapter-netlify.md">
<violation number="1" location="documentation/docs/25-build-and-deploy/80-adapter-netlify.md:17">
P2: Documentation inconsistency: The type annotation references `@tg-svelte/kit` while the adapter import and installation instructions still reference `@sveltejs/adapter-netlify`. This may confuse users - either keep the type as `@sveltejs/kit` (if using the alias approach per the PR description) or update the adapter import/installation instructions to also use the `@tg-svelte` namespace for consistency.</violation>
</file>
<file name="documentation/docs/30-advanced/10-advanced-routing.md">
<violation number="1" location="documentation/docs/30-advanced/10-advanced-routing.md:13">
P2: Inconsistent documentation: The URL example was updated to `/tg-svelte/kit/...` but the resulting parameters object still shows `org: 'sveltejs'` instead of `org: 'tg-svelte'`. The result object should match the URL example to avoid confusing readers.</violation>
</file>
<file name="documentation/docs/25-build-and-deploy/55-single-page-apps.md">
<violation number="1" location="documentation/docs/25-build-and-deploy/55-single-page-apps.md:25">
P3: Documentation inconsistency: The type annotation now references `@tg-svelte/kit`, but the same code block still shows `@sveltejs/adapter-static` for the import. Additionally, the PR's migration guide recommends npm aliasing (users would still use `@sveltejs/kit` in code). Consider either keeping `@sveltejs/kit` consistently (for alias users) or adding a note explaining when to use each approach.</violation>
</file>
<file name="documentation/docs/60-appendix/30-migrating-to-sveltekit-2.md">
<violation number="1" location="documentation/docs/60-appendix/30-migrating-to-sveltekit-2.md:14">
P2: Documentation examples should keep using `@sveltejs/kit` since the fork is meant to be installed as an alias. Users following this migration guide would still write `import { error } from '@sveltejs/kit'` in their code (the alias resolves it to the fork). Changing the docs to show `@tg-svelte/kit` is inconsistent with the recommended installation approach and will confuse users.</violation>
<violation number="2" location="documentation/docs/60-appendix/30-migrating-to-sveltekit-2.md:110">
P2: The 'before' example in this migration diff should show `@sveltejs/kit` (what users had before migrating), not `@tg-svelte/kit`. The `---` prefix indicates code being replaced, so it should reflect the original state users are migrating FROM.</violation>
</file>
<file name="documentation/docs/10-getting-started/30-project-structure.md">
<violation number="1" location="documentation/docs/10-getting-started/30-project-structure.md:90">
P3: The link reference identifier `@sveltejs-kit-vite` wasn't updated to match the new package name. The display text shows `@tg-svelte/kit/vite` but the reference still uses the old naming convention. Consider updating the reference to maintain consistency (e.g., `@tg-svelte-kit-vite` or whatever pattern the documentation system uses).</violation>
</file>
<file name="documentation/legacy/PUBLISHING_MANUAL.md">
<violation number="1" location="documentation/legacy/PUBLISHING_MANUAL.md:16">
P2: Incorrect replacement reference. The fork `@tg-svelte/kit` replaces `@sveltejs/kit`, not itself. This should remain as `@sveltejs/kit`.</violation>
<violation number="2" location="documentation/legacy/PUBLISHING_MANUAL.md:37">
P1: Broken aliasing explanation. The purpose of aliasing is to let code importing from `@sveltejs/kit` use the fork transparently. Saying 'alias to @tg-svelte/kit to import from @tg-svelte/kit' is circular and defeats the drop-in replacement strategy.</violation>
<violation number="3" location="documentation/legacy/PUBLISHING_MANUAL.md:42">
P1: Invalid npm alias command. `@tg-svelte/kit@npm:@tg-svelte/kit` is redundant and provides no aliasing. The original command aliased `@sveltejs/kit` to the fork, enabling drop-in replacement.</violation>
<violation number="4" location="documentation/legacy/PUBLISHING_MANUAL.md:49">
P1: Invalid package.json alias example. This should show `@sveltejs/kit` aliased to the fork, not a redundant self-reference.</violation>
<violation number="5" location="documentation/legacy/PUBLISHING_MANUAL.md:63">
P1: Incorrect fallback command. To fall back to standard SvelteKit, users should install `@sveltejs/kit@latest`, not the fork.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
gamesguru
force-pushed
the
legacy-builds-revived
branch
from
7a6e4d8 to
f72ea90
Compare
There was a problem hiding this comment.
3 issues found across 22 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/kit/src/exports/vite/index.js">
<violation number="1" location="packages/kit/src/exports/vite/index.js:393">
P1: The code assumes `rollupOptions.output` is an object, but Vite allows it to be an array. If output is already an array (from user config or plugins), setting properties directly on it will not work as intended. Consider checking with `Array.isArray()` and handling both cases.</violation>
</file>
<file name="packages/kit/test/apps/options-3/src/service-worker.js">
<violation number="1" location="packages/kit/test/apps/options-3/src/service-worker.js:59">
P2: Logical bug: `cached` is a Promise (always truthy), so `cached || promise` always returns `cached`, making `|| promise` dead code. If you intend to fall back only when cache has a response, you need to await and check the resolved value. Otherwise, simplify to just `cached`.</violation>
</file>
<file name="packages/kit/test/apps/options-3/src/app.html">
<violation number="1" location="packages/kit/test/apps/options-3/src/app.html:9">
P2: `display: contents` isn’t supported in IE11, so this wrapper will render as a block element in legacy builds, potentially altering layout/DOM structure. For legacy/IE11 test fixtures, avoid `display: contents` or apply it only when supported.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/kit/src/exports/vite/index.js">
<violation number="1" location="packages/kit/src/exports/vite/index.js:920">
P1: Setting `context: 'window'` unconditionally will cause issues for SSR builds where `window` is not defined. This should be conditional on the build type, using `globalThis` for SSR builds which works in both browser and Node.js environments.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/kit/src/runtime/server/page/render.js">
<violation number="1" location="packages/kit/src/runtime/server/page/render.js:394">
P1: Duplicate `env` and `assets` properties in the array. These properties appear twice, which is likely a merge/copy-paste error. The generated JavaScript object will have duplicate keys, causing unexpected behavior and bloat in the client init script.</violation>
</file>
<file name="packages/kit/test/apps/embed/src/routes/embed/+page.svelte">
<violation number="1" location="packages/kit/test/apps/embed/src/routes/embed/+page.svelte:53">
P2: The `includes()` check is broader than the regex pattern. If `document.currentScript.parentElement` appears in a different form (e.g., with `let`/`var` instead of `const`, or different variable names), the includes check passes and the container is cleared, but the regex replacement silently fails. This could leave the element reference pointing to stale DOM.
Consider using a more permissive regex or aligning the includes check with the regex pattern, similar to the existing third block that uses matching strings for both checks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Fixes TypeError: n[1] is not a function in SystemJS loader by preventing 'SvelteKitApp' name forcing on SystemJS chunks. - Only apply output.name for 'inline' bundle strategy (IIFE). - Verified with legacy-basic, test-embed, test-options suites.
- Upgraded vite, svelte, svelte-check to use catalog: (matching other tests). - Upgraded @vitejs/plugin-legacy to ^6.0.0. - Fixes CI failure due to mismatch between pinned Vite 4 and workspace vite-plugin-svelte v6.
…ill test - Upgraded dependencies to match workspace (Vite 6, etc). - Reverted vite.config.js to relative path logic (compatible with legacy). - Skipped 'test:build:only-modern-polyfill' and 'test:build:both-polyfills' because modern vite-plugin-legacy forces 'core-js/' prefix which conflicts with 'exports' in modern core-js when loading local files. Tests for no-polyfill and legacy-polyfill (primary use case) are preserved.
…full suite - Updated deps. - Restored full test suite and check commands (user request). - Implemented 'test-modern-polyfill' hijack strategy with aliases to fix build on Vite 6. - Resolves 'Rollup failed to resolve import' for custom polyfill tests.
…njection in legacy test
…talog versions (Vite 6)
There was a problem hiding this comment.
Pull request overview
This PR forks SvelteKit under @tg-svelte/kit and introduces legacy-browser build support (including @vitejs/plugin-legacy compatibility) by adjusting Vite build output handling and client init/hydration behavior, along with CI and documentation updates.
Changes:
- Renames the framework package and associated imports/docs from
@sveltejs/kitto@tg-svelte/kit. - Updates Kit’s Vite plugin + runtime init/hydration paths to support legacy/modern multi-output builds and inline/IIFE strategies.
- Adds legacy build verification CI (with a legacy template submodule) and legacy-support documentation.
Reviewed changes
Copilot reviewed 280 out of 563 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/kit/src/exports/vite/index.js | Adds legacy-aware build output handling (multi-output, inline/IIFE) and adjusts Rollup output settings. |
| packages/kit/src/runtime/server/page/render.js | Updates client init script generation (service worker options, route data serialization, legacy init compatibility). |
| packages/kit/src/runtime/client/parse.js | Enhances server-route parsing to support CSR-disabled flows by executing route matching client-side using matchers. |
| packages/kit/src/runtime/server/page/index.js | Adds loader type checks before invoking SSR node loaders. |
| .github/workflows/test-legacy-build.yml | New CI workflow to build Kit and run E2E tests against a legacy template across OSes. |
| package.json | Renames monorepo metadata and expands the root build filter to include @tg-svelte/*. |
| packages/kit/package.json | Renames package to @tg-svelte/kit, bumps version to 2.49.4-rc.0, and adjusts build/check scripts. |
| documentation/legacy/GUIDE_LEGACY_SUPPORT.md | Adds user-facing instructions for enabling legacy browser support with @vitejs/plugin-legacy. |
| documentation/legacy/PUBLISHING_MANUAL.md | Adds publishing instructions for the forked package under a custom npm scope. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/kit/src/runtime/server/page/render.js">
<violation number="1" location="packages/kit/src/runtime/server/page/render.js:521">
P1: `globalThis` is undefined in IE11, so this inline import path will throw in legacy builds. Use a safe global fallback (e.g., window) to maintain IE11 compatibility.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const path = s(prefixed(getPathFunc(asset))); | ||
| const imp = `${import_func}(${path})`; | ||
| if (client.inline && asset.file === client.start) { | ||
| return `${imp}.then(function() { return globalThis.${global}.app })`; |
There was a problem hiding this comment.
P1: globalThis is undefined in IE11, so this inline import path will throw in legacy builds. Use a safe global fallback (e.g., window) to maintain IE11 compatibility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/kit/src/runtime/server/page/render.js, line 521:
<comment>`globalThis` is undefined in IE11, so this inline import path will throw in legacy builds. Use a safe global fallback (e.g., window) to maintain IE11 compatibility.</comment>
<file context>
@@ -518,7 +518,7 @@ export async function render_response({
const imp = `${import_func}(${path})`;
if (client.inline && asset.file === client.start) {
- return `${imp}.then(function() { return ${global}.app })`;
+ return `${imp}.then(function() { return globalThis.${global}.app })`;
}
return imp;
</file context>
Suggested change
| return `${imp}.then(function() { return globalThis.${global}.app })`; | |
| return `${imp}.then(function() { return (typeof globalThis !== 'undefined' ? globalThis : window).${global}.app })`; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@cubic-dev-ai review this pull request
Summary by cubic
Adds legacy browser build support to SvelteKit by handling Vite’s legacy multi-output and single-bundle strategies and entry resolution, and ships a forked package under @tg-svelte/kit with CI and docs. Also fixes the client init script (service worker options, streaming app imports, serialization order) to ensure stable static builds with @vitejs/plugin-legacy, including IE11, and resolves a SystemJS chunk naming collision.
New Features
Migration
Written for commit 93c58ee. Summary will update on new commits.